Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 332 abc data #427

Closed
wants to merge 1 commit into from

Conversation

emfdavid
Copy link
Contributor

@emfdavid emfdavid commented Aug 31, 2021

Resolves #332

Register ABCMeta to use StockPickler. If workable, this is preferable to the cloudpickle solution that depends heavily on the internals of the ABC implementation.

This will remain broken on some versions of Python 3.7 that did not support pickling the new c implementation of ABC.

Issues:

  • Registering ABCMeta seems to exclude other registered types, for instance the module/locality solution for ClassType (see broken test)
  • Need to confirm expected behavior for ABC registry once locality issue is solved

@mmckerns
Copy link
Member

you may want to check against any of these tests that are relevant: https://github.com/cloudpipe/cloudpickle/blob/343da119685f622da2d1658ef7b3e2516a01817f/tests/cloudpickle_test.py#L1170

@mmckerns
Copy link
Member

Can you give a short summary of what you feel is done and what is left to do? I'm heading toward a release, and trying to get some nagging issues/PRs tied up. I'll help with this if possible.

@emfdavid
Copy link
Contributor Author

emfdavid commented Oct 19, 2021

@mmckerns Status update
I spent a bit more time on this today, but it is still not complete.
There seem to be three remaining issues:

  1. The implementation for <locals> is not satisfactory. The class path is not the same for the deserialized objects when they are locals rather than normal module objects.
  2. The register function does not behave as expected based on the tests and issues observed in CloudPickle.
  3. The current implementation does not work for python < 3.6. I believe the if registry is None clause of the cloud pickle implementation handles the post/pre 3.6 implementation change.

I don't think anything that previously worked would be broken by these changes, but I have not confirmed that ABC classes failed to pickle in python <= 3.6

@anivegesana
Copy link
Contributor

@mmckerns @emfdavid Take a look at https://github.com/anivegesana/dill/tree/issue_332_abc_data and see if it is going in the direction that you want it.

@mmckerns
Copy link
Member

mmckerns commented Jan 2, 2022

@mmckerns @emfdavid Take a look at https://github.com/anivegesana/dill/tree/issue_332_abc_data and see if it is going in the direction that you want it.

@anivegesana: Yeah, this is a good direction.

@anivegesana
Copy link
Contributor

@emfdavid Bump

@mmckerns
Copy link
Member

@anivegesana: feel free to assume the answer is yes, as opposed to waiting.

@emfdavid
Copy link
Contributor Author

emfdavid commented Jan 19, 2022 via email

@emfdavid
Copy link
Contributor Author

Very cool @anivegesana - 2ce1c9e looks really good.
Tests are much improved using copy instead of dump and load and the implementation is cleaner. I think I can see how you are properly handling the registries now too.
Please don't block on me from here. Thank you!

@emfdavid emfdavid closed this Jan 20, 2022
@emfdavid emfdavid deleted the issue_332_abc_data branch January 20, 2022 03:22
@mmckerns mmckerns added this to the dill-0.3.5 milestone Apr 22, 2022
anivegesana pushed a commit to anivegesana/dill that referenced this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: can't pickle _abc_data objects
3 participants